Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Precommit: run mypy on all files, including tests. #283

Merged
merged 66 commits into from
May 27, 2024

Conversation

fzimmermann89
Copy link
Member

@fzimmermann89 fzimmermann89 commented May 12, 2024

Runs mypy on all files. As changes to one file can cause typing issues in unchanged files.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@fzimmermann89
Copy link
Member Author

fzimmermann89 commented May 12, 2024

Copy link
Contributor

github-actions bot commented May 12, 2024

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/mrpro/algorithms/csm
   _iterative_walsh.py15193%34
src/mrpro/algorithms/optimizers
   _adam.py17194%82
src/mrpro/algorithms/reconstruction
   _direct_reconstruction.py633643%53–58, 75–83, 95–97, 113–120, 134–145
   _reconstruction.py10190%20
src/mrpro/data
   _AcqInfo.py83298%100, 138
   _CsmData.py21386%28, 72–74
   _DcfData.py91595%36, 42, 86, 111, 199
   _IData.py61395%136, 142, 146
   _IHeader.py59297%80, 107
   _KHeader.py127993%37, 84, 122, 167, 178, 185–186, 189, 196
   _KNoise.py22959%54–67
   _MoveDataMixin.py1331489%36, 130, 146, 160–162, 223, 286, 300, 379, 399–400, 417–418
   _QData.py31197%56
   _SpatialDimension.py46198%80
   _TrajectoryDescription.py14193%38
src/mrpro/data/_kdata
   _KData.py108794%104–105, 175–176, 218, 223–224
   _KDataRemoveOsMixin.py30293%53, 55
   _KDataSelectMixin.py22291%62, 78
   _KDataSplitMixin.py51394%67, 97, 106
src/mrpro/data/traj_calculators
   _KTrajectoryCalculator.py27293%40, 62
   _KTrajectoryIsmrmrd.py16288%58, 67
   _KTrajectoryPulseq.py30197%69
src/mrpro/operators
   _CartesianSamplingOp.py50982%63–64, 69–70, 75–76, 102, 105, 128
   _ConstraintsOp.py61297%38, 40
   _EndomorphOperator.py57395%219, 223, 227
   _FastFourierOp.py31294%84, 94
   _FourierOp.py78199%146
   _GridSamplingOp.py123993%74–75, 84–85, 92–93, 96, 98, 100
   _LinearOperator.py64395%41, 142, 147
   _Operator.py56198%38
   _SliceProjectionOp.py168895%55, 62, 64, 70, 192, 213, 246, 286
   _ZeroPadOp.py16194%44
src/mrpro/utils
   _Rotation.py4573093%60–70, 110, 287, 373, 375, 388, 445, 450, 453, 468, 484, 488, 633, 638, 641, 656, 659, 734, 736, 744–745, 985, 1067
   _split_idx.py10280%57, 61
   _zero_pad_or_crop.py31681%40, 44, 68, 71, 74, 77
   filters.py68691%48–50, 75, 80, 87
   slice_profiles.py45687%31, 47, 124–127, 160
   sliding_window.py35197%47
TOTAL320719894% 

Tests Skipped Failures Errors Time
656 0 💤 0 ❌ 0 🔥 1m 0s ⏱️

@fzimmermann89
Copy link
Member Author

As changes to a file can also lead to type errors in other files, only checking committed files does not help.
Also, a lot of non-commited files will be parsed by mypy anyways, if they are imported. Overall, mypy suggested to run on all files
python/mypy#13916

As #282 fixes all remaining issues in the tests, we can now also check the tests.

@fzimmermann89 fzimmermann89 requested a review from schuenke May 12, 2024 12:59
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@fzimmermann89
Copy link
Member Author

fzimmermann89 commented May 12, 2024

Mypy cannot infer that the nullcontext or pytest.warns condition always results in a context manager.

This either needs a type hint or a different logic.

python/mypy#10109

--> fixed in #282

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@fzimmermann89
Copy link
Member Author

fzimmermann89 commented May 22, 2024

The only issue now is, that if you have some non-staged files with typing issues in the src directory,
the precommit will fail...

not sure if there is an easy way to run on 'staged and already committed files' in precommit..

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
fzimmermann89 added a commit that referenced this pull request May 27, 2024
ghstack-source-id: 0f7ce8094f8f592286fe8c0a2193dac831881d50
ghstack-comment-id: 2106237133
Pull Request resolved: #283
Base automatically changed from gh/fzimmermann89/25/head to main May 27, 2024 20:45
@fzimmermann89 fzimmermann89 merged commit 5540525 into main May 27, 2024
15 checks passed
@fzimmermann89 fzimmermann89 deleted the gh/fzimmermann89/26/head branch May 27, 2024 20:51
fzimmermann89 added a commit that referenced this pull request Nov 10, 2024
Changes to one file can cause mypy failures in non-changed files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants